Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Allow storage different from CDN to be tested by RegistrationComparer #749

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

joelverhagen
Copy link
Member

@joelverhagen joelverhagen commented Feb 24, 2020

Improved logging around HTTP requests.
Removed duplicate HTTP request (OMG).
Moved response body fetching into HttpClient.GetAsync so HttpClient.Timeout acts on it.

Right now, this job can only test China storage if it's running from China. This change enables hitting storage that doesn't sit behind the client's resolution of api.nuget.org.

Related to NuGet/NuGetGallery#7741.

.SelectMany(x => x)
.Concat(leafUrlGroups.SelectMany(x => x)));
.Concat(leafUrlGroups.SelectMany(x => x))
.Select(x => new KeyValuePair<string, HiveConfiguration>(x.Url, x.Hive)));
Copy link
Contributor

@agr agr Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KeyValuePair.Create(x.Url, x.Hive) would be a bit more compact.
What is Url here, by the way? Is it just to be able to throw exception in GetJObjectOrNullAsync? If yes, given that it is a private method, can we validate data earlier?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the official (not blob storage) URL of a registration leaf that needs to be downloaded and validated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The URL is there so we can parallelize the downloads of many leaves and pages.

I'll use the KeyValuePair.Create trick.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhhyu
Copy link
Contributor

zhhyu commented Feb 24, 2020

No need to update the test coverage?

@joelverhagen
Copy link
Member Author

No need to update the test coverage?

This is a validation job for the one-time transition from old registration to new registration impl. I did not write unit tests for this level.

@joelverhagen joelverhagen merged commit b39a78c into dev Feb 25, 2020
@joelverhagen joelverhagen deleted the jver-cmp branch February 25, 2020 18:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants